Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[uss_qualifier] DSS0030 slight overlap subscription interactions #360

Merged

Conversation

Shastick
Copy link
Contributor

Extends ISASubscriptionInteractions generic scenario to include a subscription that only barely intersects with the ISA.

Note that this subscription's area is generated based on the configured ISA's area (see generate_slight_overlap_area in common/dss/utils.py).

For ISA footprints that are a square or rectangle, the new test area is guaranteed to only share a single point with the footprint: if this is too restrictive, we can increase the overlap area.

If required, this new test area could also be added to the configuration instead of being generated, but we would then risk users to configure an area that would not respect the spirit of the test.

@Shastick Shastick marked this pull request as ready for review November 22, 2023 11:29
@Shastick Shastick force-pushed the dss0030-isa-sub-slight-overlap branch 2 times, most recently from 54a155f to 2abe74a Compare November 27, 2023 09:56
Copy link
Member

@BenjaminPelletier BenjaminPelletier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good -- just 2 comments

@@ -35,13 +35,15 @@ def __init__(
# sub id is isa_id with last character replaced with '1'
# (the generated isa_id ends with a few '0's)
self._sub_id = self._isa_id[:-1] + "1"
self._slight_overlap_sub_id = self._isa_id[:-1] + "2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a separate ID?

Copy link
Contributor Author

@Shastick Shastick Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly, my considerations:

  • originally I thought of creating a separate scenario, but then piggy-backed on this one instead. I then stuck with "I need a new subscription ID for it"
  • I've thought of cleaning it up and re-creating it (or alternatively, to mutate the existing subscription), but I feel that this might miss cases where the DSS implementation deals with mutations or deletions in a weird way and would have this test pass when a subscription with a new ID might not?
  • I don't know what is a more representative use case: creating a subscription on the border of the ISA, or mutating an existing one to touch the ISA instead.

Let me know if I'm over- or rather under-thinking this: the above questions make me think of additional scenario tests cases that would actually be worth doing: I can make myself a note to revisit this as a P3, or earlier if you think it is worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original intent of having special IDs (via the ID generator) was to make it easy to figure out who left orphaned test objects in the system, and what those test objects were being used for (how they came to be). IDs generated by the IDGenerator can be decoded to determine the owner and resource type (test purpose). I had missed creation of an ad-hoc ID above (which would not fulfill that contract).

I would generally expect just one Subscription to be sufficient to test ISA-Subscription interactions since I would expect that we could usually delete a previous Subscription before testing new Subscription behavior (there would be no need to have two Subscriptions exist simultaneously to test ISA-Subscription interactions). To the extent that's true, I would expect only one Subscription ID. If that assumption isn't true, then of course we should use as many IDs as necessary, though I would be interested to know the thing we're testing that requires two simultaneous Subscriptions. The thing you mention in your second bullet seems like it could qualify.

I don't know what is a more representative use case: creating a subscription on the border of the ISA, or mutating an existing one to touch the ISA instead.

Those do both seem like valid, expected, and separate use cases so it would probably be worth covering both eventually. That level of full coverage on these DSS functions is probably not the highest priority immediately though, so I'd say pick whichever one is easier and put a TODO and/or Issue for the other one (unless doing both now is easy and would be harder later, e.g., due to mentally re-caching).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll use a single ID and update the scenario accordingly. As I have all the required pieces available to test the mutation as well as the "newly created" cases I'll implement them at the same time.

Copy link
Contributor Author

@Shastick Shastick Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought: there is a whole bunch of additional things that we can reasonably do, so I left a TODO to that effect. The possibility of defining and reusing test fragments (as proposed in #380) would also help making the scenario and its documentation more compact if/when we choose to extend it

@Shastick Shastick marked this pull request as draft November 29, 2023 13:05
@Shastick
Copy link
Contributor Author

Parked while I look at #369 and until I addressed the comments.

@BenjaminPelletier in the meantime I'd be happy to get your input on whether to extend the scenario or not or to keep the separate ID or not: if there are reasons for avoiding using new IDs unless strictly necessary I'd be happy to know them for the future scenarios I'll write.

@Shastick Shastick force-pushed the dss0030-isa-sub-slight-overlap branch 2 times, most recently from 02b4a0e to 4a3b372 Compare November 30, 2023 10:54
@Shastick
Copy link
Contributor Author

Moved the relevant function to geo.py.

@Shastick Shastick marked this pull request as ready for review November 30, 2023 11:00
@Shastick Shastick force-pushed the dss0030-isa-sub-slight-overlap branch from 4a3b372 to e93e2e0 Compare December 1, 2023 09:22
@Shastick Shastick force-pushed the dss0030-isa-sub-slight-overlap branch from e93e2e0 to 47b4655 Compare December 1, 2023 09:35
@BenjaminPelletier BenjaminPelletier merged commit 0f4b4d4 into interuss:main Dec 1, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants